Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix various iteration issues #296

Merged
merged 1 commit into from
Jul 26, 2021
Merged

fix various iteration issues #296

merged 1 commit into from
Jul 26, 2021

Conversation

pfitzseb
Copy link
Member

This fixes two iteration order issues (kwargs with multiple ;s and things like :(;;;)) I introduced in #291.
Also tests whether iteration over any of the test expressions in test/parser.jl throws an error.

@pfitzseb pfitzseb requested a review from a team July 26, 2021 18:11
@davidanthoff davidanthoff merged commit 3aa5133 into master Jul 26, 2021
@davidanthoff davidanthoff deleted the sp/iter-fixes branch July 26, 2021 18:22
Comment on lines +198 to +211
elseif x.head === :parameters
if length(x.args) > 1 && any(a -> a.head === :parameters, x.args)
ordered_args = EXPR[]
for arg in x.args
if arg.head === :parameters
pushfirst!(ordered_args, arg)
else
push!(ordered_args, arg)
end
end
Expr(:parameters, Expr.(ordered_args)...)
else
Expr(:parameters, Expr.(x.args)...)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a priority right now but we shouldn't need to special case conversion so should look to fix the underlying inconsistency w/ the scheme parser

Copy link
Member Author

@pfitzseb pfitzseb Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's basically what I reverted here. Originally I had the right order in the EXPR, but that failed during iterate, so this was easier for me than wrapping my head around how iteration works :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants